Conversation
31de9a6 to
362196a
Compare
changed type to reuse test case assignment interface fix for case when input has no variable name
…st result cases yet
backend working? frontend not yet for error cases
… backend compplete
…object for each error case
fix + refactor slightly on backend for grouping test result
395c6e4 to
dfac775
Compare
There was a problem hiding this comment.
Can we use luicide-react for the icons?
| s.src = chrome.runtime.getURL("proxy.js"); | ||
| s.type = "text/javascript"; | ||
| s.onload = () => s.remove(); | ||
| document.documentElement.appendChild(s); |
There was a problem hiding this comment.
can you clarify what this is for?
| const regexTestResult = | ||
| /^https:\/\/leetcode\.com\/submissions\/detail\/runcode_[^/]+\/check\/$/; | ||
|
|
||
| if (typeof res.url === "string" && regexTestResult.test(res.url)) { |
There was a problem hiding this comment.
| if (typeof res.url === "string" && regexTestResult.test(res.url)) { | |
| if (regexTestResult.test(res.url)) { |
|
|
||
| const origFetch = window.fetch; | ||
|
|
||
| window.fetch = async (...args) => { |
| { | ||
| source: "LC_HOOK", | ||
| url: res.url, | ||
| data: data, | ||
| type: "LC_HOOK", | ||
| }, |
There was a problem hiding this comment.
Worth adding a new type here https://github.com/nickbar01234/codebuddy/blob/main/extension/src/types/window.ts.
In https://github.com/nickbar01234/codebuddy/pull/549/changes#diff-e34ae516552cad6954a715c3177eb0e99778fa92ad88556a1d574eb9d88014a0R167-R174, we can do a strongly typed handler
const messageHandler = async (event: MessageEvent<LeetCodeHook>) => {There was a problem hiding this comment.
nit: LC_SUBMISSION_RESULT instead of LC_HOOK?
|
Can you comment with the latest screenshot of the new UI? |
|
|
||
| export const getTestResultsPayload = ( | ||
| variables: Question["variables"] | undefined, | ||
| testResults?: any |
There was a problem hiding this comment.
This can be strongly typed right
| { | ||
| value: "testResult", | ||
| label: "Test Result", | ||
| Icon: FlaskConical, |
There was a problem hiding this comment.
Can we change the icon to https://lucide.dev/icons/terminal
| const passed = (test.testResult ?? []).every( | ||
| (r: ResultAssignment) => r.output === r.expected | ||
| ); | ||
| const selected = !!test.selected; |
There was a problem hiding this comment.
This is not necessary right?
| return ( | ||
| <div key={idx} onClick={() => selectTestResult(idx)}> | ||
| <button | ||
| className={`${baseClasses} ${selected ? selectedClasses : unselectedClasses} gap-2`} |
There was a problem hiding this comment.
| className={`${baseClasses} ${selected ? selectedClasses : unselectedClasses} gap-2`} | |
| className={cn(baseClasses, { selectedClasses: selected, unselectedClasses: !selected )} |
| inputs: { variable?: string; value: string }[]; | ||
| } | ||
|
|
||
| export const InputDisplay: React.FC<InputDisplayProps> = ({ inputs }) => ( |
There was a problem hiding this comment.
Haven't looked through all the css yet but let's make sure the styles match with TestTab
| @@ -0,0 +1,13 @@ | |||
| import React from "react"; | |||
There was a problem hiding this comment.
I would rename testResultComponents to TestResults
| @@ -0,0 +1,112 @@ | |||
| import { SelectableTestResult } from "@cb/types"; | |||
There was a problem hiding this comment.
maybe sharedComponents.tsx as base.tsx?
| export const TEST_RESULT_ERROR = { | ||
| "Runtime Error": "full_runtime_error", | ||
| "Compile Error": "full_compile_error", | ||
| "Invalid Test Case": "full_runtime_error", |
There was a problem hiding this comment.
should we consolidate the constant here with https://github.com/nickbar01234/codebuddy/pull/549/changes#diff-ad1d6f906f1d151b1f3d7a34cc0286a0b2520920a2f9059638c878cac7c55911R3-R10?
| } | ||
|
|
||
| export interface TestResult { | ||
| testResultStatus: string; |
There was a problem hiding this comment.
type is union of all keys https://github.com/nickbar01234/codebuddy/pull/549/changes#diff-d9cc01c7b64c1e15627e55b2ebfefe0f214fe3306afb06537b1f1478bc6ffdd4R116 and unknown for cases we don't handle yet
| testResults.code_answer?.slice(0, -1), | ||
| testResults.expected_code_answer?.slice(0, -1), |
There was a problem hiding this comment.
can you clarify why we exclude last idx
|
|
||
| const getLastRunIndex = () => | ||
| codeAnswer.findIndex((val) => val !== "0") === -1 | ||
| ? codeAnswer.length |
There was a problem hiding this comment.
wouldn't this be out of bound? Also we throw away some work with 2 findIndex
| const getFirstInput = () => { | ||
| if (numCases === 0) return []; | ||
| const firstTest = testInputs[0]; | ||
| if (firstTest.test.length !== varCount) return []; |
There was a problem hiding this comment.
Instead of multiple checks throughout the function, how do you feel about doing all these checks upfront and returning empty list otherwise? It just makes the code easier to read if we focus only on the logic
| } | ||
|
|
||
| // Because statusMessage of invalid test case is the same as runtime error although they are handled differently | ||
| const statusMsg: string = testResults.invalid_testcase |
There was a problem hiding this comment.
A bit confuse why we need both statusMsg and https://github.com/nickbar01234/codebuddy/pull/549/changes#diff-69f1956d4fa6ee2face76261231445546a6bc5bcc1d528c82300843c32cd1cfdR48-R52
|
|
||
| export const getTestResultsPayload = ( | ||
| variables: Question["variables"] | undefined, | ||
| testResults?: any |
There was a problem hiding this comment.
would you be able to paste a couple examples of what testResults is for each of the cases you're targeting?
| ); | ||
| const overallStatus = allMatch ? "Accepted" : "Wrong Answer"; | ||
|
|
||
| // Only loop for Accepted cases |
There was a problem hiding this comment.
this loops for everything though right
| }); | ||
| } | ||
|
|
||
| return results.length > 0 ? results : [{ testResultStatus, testResult: [] }]; |
There was a problem hiding this comment.
we should always be able to return just results?
| }; | ||
|
|
||
| return ( | ||
| <SkeletonWrapper loading={false} className="relative"> |
There was a problem hiding this comment.
Can we wrap this in error boundary? https://react.dev/reference/react/Component#catching-rendering-errors-with-an-error-boundary. Reasoning is that this feature is quite complex with a lot of array indexing and handling, so we don't want to break other working components
|
|
||
| export type TestCases = TestCase[]; | ||
|
|
||
| export type TestResults = TestResult[]; |
There was a problem hiding this comment.
hmm, the use of TestResults is a bit inconsistent. In https://github.com/nickbar01234/codebuddy/pull/549/changes#diff-a6c0609a405d5d0726edb4d54dd9ddaab92ce2a9c7224a405da9e0bda60b5d61R32, runtime errors return 1-item list. For cases where we run the tests, the length matches the number of test input, with each TestResult.testResult being 1 element.
It looks to me like we actually just need TestResult to capture everything
Description
Async test result when one buddy click Run button of Leetcode
Screenshots
Test
Click Run code on one buddy and wait a bit to see the Test Result tab filled out on the other.
Checklist
If you're making changes to the extension, please run through the following checklist to make sure that we don't have
any regressions. Note that we plan to add integration tests in the future!
Possible Downsides
Additional Documentations